Skip to content

Avoid deadlocks after fork - #303

Open
danth wants to merge 5 commits into
eclipse-score:mainfrom
etas-contrib:avoid-deadlocks-after-fork
Open

Avoid deadlocks after fork#303
danth wants to merge 5 commits into
eclipse-score:mainfrom
etas-contrib:avoid-deadlocks-after-fork

Conversation

@danth

@danth danth commented Jul 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Fork only copies the current thread, so from that point on we could deadlock if we access anything that was locked by another thread at the time of the fork.

According to man 2 fork we should only use async signal safe functions between fork and execve.

@danth
danth requested a deployment to workflow-approval July 9, 2026 12:31 — with GitHub Actions Waiting
@danth
danth requested a deployment to workflow-approval July 9, 2026 12:31 — with GitHub Actions Waiting
@danth danth mentioned this pull request Jul 9, 2026
Open
2 tasks
@danth

This comment was marked as outdated.

Sign in to view
@danth
danth force-pushed the avoid-deadlocks-after-fork branch from bcb913a to 2a70190 Compare July 10, 2026 08:09
@danth
danth requested a deployment to workflow-approval July 10, 2026 08:09 — with GitHub Actions Waiting
@danth
danth requested a deployment to workflow-approval July 10, 2026 08:09 — with GitHub Actions Waiting
@github-actions

github-actions Bot commented Jul 10, 2026 •
edited
Loading

Copy link
Copy Markdown

License Check Results

🚀 The license check job ran with the Bazel command:

bazel run --lockfile_mode=error //:license-check

Status: ⚠️ Needs Review

Click to expand output 
[License Check Output]
Extracting Bazel installation...
Starting local Bazel server (8.6.0) and connecting to it...
INFO: Invocation ID: 7938ef53-47e8-4fb3-a674-12fb796df7a7
Computing main repo mapping: 
Computing main repo mapping: 
Loading: 
Loading: 0 packages loaded
Loading: 0 packages loaded
Loading: 0 packages loaded
    currently loading: 
Loading: 0 packages loaded
    currently loading: 
Loading: 0 packages loaded
    currently loading: 
Loading: 0 packages loaded
    currently loading: 
Loading: 0 packages loaded
    currently loading: 
Loading: 0 packages loaded
    currently loading: 
Analyzing: target //:license-check (1 packages loaded, 0 targets configured)
Analyzing: target //:license-check (1 packages loaded, 0 targets configured)

Analyzing: target //:license-check (35 packages loaded, 10 targets configured)

Analyzing: target //:license-check (89 packages loaded, 10 targets configured)

Analyzing: target //:license-check (145 packages loaded, 2362 targets configured)

Analyzing: target //:license-check (157 packages loaded, 5984 targets configured)

Analyzing: target //:license-check (158 packages loaded, 8564 targets configured)

Analyzing: target //:license-check (170 packages loaded, 9397 targets configured)

Analyzing: target //:license-check (170 packages loaded, 9397 targets configured)

Analyzing: target //:license-check (170 packages loaded, 9397 targets configured)

Analyzing: target //:license-check (170 packages loaded, 9397 targets configured)

Analyzing: target //:license-check (174 packages loaded, 11409 targets configured)

INFO: Analyzed target //:license-check (175 packages loaded, 11535 targets configured).
[7 / 16] [Prepa] Creating source manifest for @@score_tooling+//dash/tool/formatters:dash_format_converter [for tool]
[12 / 16] JavaToolchainCompileClasses external/rules_java+/toolchains/platformclasspath_classes; 0s disk-cache, processwrapper-sandbox ... (2 actions running)
[14 / 16] JavaToolchainCompileBootClasspath external/rules_java+/toolchains/platformclasspath.jar; 0s disk-cache, processwrapper-sandbox
INFO: Found 1 target...
Target //:license.check.license_check up-to-date:
  bazel-bin/license.check.license_check
  bazel-bin/license.check.license_check.jar
INFO: Elapsed time: 27.144s, Critical Path: 2.59s
INFO: 16 processes: 12 internal, 3 processwrapper-sandbox, 1 worker.
INFO: Build completed successfully, 16 total actions
INFO: Running command line: bazel-bin/license.check.license_check ./formatted.txt <args omitted>
usage: org.eclipse.dash.licenses.cli.Main [-batch <int>] [-cd <url>]
       [-confidence <int>] [-ef <url>] [-excludeSources <sources>] [-help] [-lic
       <url>] [-project <shortname>] [-repo <url>] [-review] [-summary <file>]
       [-timeout <seconds>] [-token <token>]

@github-actions

github-actions Bot commented Jul 10, 2026

Copy link
Copy Markdown

The created documentation from the pull request is available at: docu-html

@danth
danth force-pushed the avoid-deadlocks-after-fork branch from 2a70190 to 9cc08d6 Compare July 10, 2026 08:22
@danth
danth requested a deployment to workflow-approval July 10, 2026 08:22 — with GitHub Actions Waiting
@danth
danth requested a deployment to workflow-approval July 10, 2026 08:22 — with GitHub Actions Waiting
MaciejKaszynski
MaciejKaszynski requested changes Jul 14, 2026
View reviewed changes
Comment thread score/launch_manager/src/daemon/src/process_group_manager/details/configure_process.cpp Outdated
Comment thread score/launch_manager/src/daemon/src/process_group_manager/details/configure_process.cpp Outdated
Comment thread score/launch_manager/src/daemon/src/process_group_manager/details/configure_process.cpp Outdated
Comment thread score/launch_manager/src/daemon/src/process_group_manager/details/configure_process.cpp Outdated
Comment thread score/launch_manager/src/daemon/src/process_group_manager/details/process_group_manager.cpp Outdated
@danth
danth force-pushed the avoid-deadlocks-after-fork branch from 9cc08d6 to 67c0d0f Compare July 14, 2026 09:57
@danth
danth requested a deployment to workflow-approval July 14, 2026 09:57 — with GitHub Actions Waiting
@danth
danth requested a deployment to workflow-approval July 14, 2026 09:57 — with GitHub Actions Waiting
@danth
danth force-pushed the avoid-deadlocks-after-fork branch from 67c0d0f to 9c28979 Compare July 14, 2026 10:05
@danth
danth requested a deployment to workflow-approval July 14, 2026 10:05 — with GitHub Actions Waiting
@danth
danth requested a deployment to workflow-approval July 14, 2026 10:05 — with GitHub Actions Waiting
@danth
danth requested a deployment to workflow-approval July 14, 2026 10:37 — with GitHub Actions Waiting
@danth
danth requested a deployment to workflow-approval July 14, 2026 10:37 — with GitHub Actions Waiting
@danth
danth force-pushed the avoid-deadlocks-after-fork branch from e249b48 to 4121230 Compare July 15, 2026 10:33
@danth
danth requested a deployment to workflow-approval July 28, 2026 12:57 — with GitHub Actions Waiting
@danth

danth commented Jul 28, 2026

Copy link
Copy Markdown
Member Author

New implementation considering previous feedback:

  • Use a separate function, not the assertion handler, so it cannot be swapped out for something unsafe
  • Use signal safe implementations of strerror, otherwise fall back to printing the raw error number
  • Build the error message in a fixed size buffer to avoid allocations and multiple writes
  • Comments on all affected functions to avoid later changes making this unsafe again
  • Minimal diff to aid review

@danth
danth force-pushed the avoid-deadlocks-after-fork branch from ea37191 to 2026010 Compare July 28, 2026 13:30
@danth
danth requested a deployment to workflow-approval July 28, 2026 13:30 — with GitHub Actions Waiting
@danth
danth requested a deployment to workflow-approval July 28, 2026 13:30 — with GitHub Actions Waiting
@danth
danth force-pushed the avoid-deadlocks-after-fork branch from 2026010 to 991ccd9 Compare July 29, 2026 08:29
@danth
danth requested a deployment to workflow-approval July 29, 2026 08:29 — with GitHub Actions Waiting
@danth
danth requested a deployment to workflow-approval July 29, 2026 08:29 — with GitHub Actions Waiting
MaciejKaszynski
MaciejKaszynski requested changes Jul 29, 2026
View reviewed changes

@MaciejKaszynski MaciejKaszynski left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All looks good, think we should however also include a unit test just so coverage doesn't go down.

Look at https://stackoverflow.com/questions/3803465/how-to-capture-stdout-stderr-with-googletest for info how to replace the stderr and it should be quite simple to cover this all.

NicolasFussberger
NicolasFussberger reviewed Jul 29, 2026
View reviewed changes
Comment thread score/launch_manager/src/daemon/src/common/signal_safe_log.hpp Outdated
NicolasFussberger
NicolasFussberger reviewed Jul 29, 2026
View reviewed changes
Comment thread score/launch_manager/src/daemon/src/common/signal_safe_log_UT.cpp
paulquiring
paulquiring reviewed Jul 29, 2026
View reviewed changes
Comment thread score/launch_manager/src/daemon/src/common/signal_safe_log.hpp Outdated
paulquiring
paulquiring reviewed Jul 29, 2026
View reviewed changes
Comment thread score/launch_manager/src/daemon/src/common/signal_safe_log.hpp Outdated
paulquiring
paulquiring reviewed Jul 29, 2026
View reviewed changes
Comment thread score/launch_manager/src/daemon/src/common/signal_safe_log.hpp Outdated
paulquiring
paulquiring reviewed Jul 29, 2026
View reviewed changes
Comment thread score/launch_manager/src/daemon/src/common/signal_safe_log_UT.cpp
@danth danth mentioned this pull request Jul 29, 2026
Open
danth and others added 5 commits July 30, 2026 09:11
@danth
Add missing sysexit in handleComms
172ff5b
@danth @MaciejKaszynski
Implement async signal safe logging utility
4ac9cc1 
Co-authored-by: Maciej Kaszynski <161459353+MaciejKaszynski@users.noreply.github.com>
@danth
Replace log calls with signal safe version after forking
5537511
@danth
Add comments about signal safety
5dbf471
@danth @MaciejKaszynski
Add unit tests for signal_safe_log
5543689 
Co-authored-by: Maciej Kaszynski <161459353+MaciejKaszynski@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@NicolasFussberger NicolasFussberger NicolasFussberger left review comments
@MaciejKaszynski MaciejKaszynski MaciejKaszynski left review comments
@pawelrutkaq pawelrutkaq Awaiting requested review from pawelrutkaq pawelrutkaq is a code owner
@ramceb ramceb Awaiting requested review from ramceb ramceb is a code owner
@FScholPer FScholPer Awaiting requested review from FScholPer FScholPer is a code owner
@antonkri antonkri Awaiting requested review from antonkri antonkri is a code owner
@anmittag anmittag Awaiting requested review from anmittag anmittag is a code owner
+1 more reviewer
@paulquiring paulquiring paulquiring left review comments
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

@danth danth

Labels

None yet

Projects

LCM - Lifecycle & Health FT
Status: Ready

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@danth @paulquiring @anmittag @NicolasFussberger @MaciejKaszynski